Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.70%. Comparing base (e72f23a) to head (106bd66).
⚠️ Report is 38 commits behind head on devel.

@@            Coverage Diff             @@
##            devel    #1411      +/-   ##
==========================================
- Coverage   55.95%   55.70%   -0.26%     
==========================================
  Files          60       63       +3     
  Lines        9956    10529     +573     
==========================================
+ Hits         5571     5865     +294     
- Misses       4095     4371     +276     
- Partials      290      293       +3     

... and 6 files with indirect coverage changes

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

With these PRs for the escalation's we have now recreated the logic of PodContainerHealthy(). I think it is time to look at calling this function instead of leaving the logic in RunWorkWithLogger()

Thank you for your review @lranjbar. I agree that it is a better practice to use a specific helper like PodContainerHealthy() and I see that it was added in #1371 over a month ago but was not discussed for this effort.

I've been engaged in this review and #1400, so I took a look at PodContainerHealthy to see how it could fit here. While it's a good abstraction of our operational logic prior to #1400 (with some additional checks), it's not a drop-in replacement for the logic that has been iterated on, tested, and reviewed in #1400 / #1411. At a glance, there are differences to handling exit codes and Ready that don't align. These are areas the team has focused a great deal on, and they are critical to our response here.

Our priority with this PR is to get our updated logic in a build out for testing and feedback (and be ready to incorporate that feedback). I'm totally onboard with refactoring after we confirm a fix, but before is premature in my opinion. So I don't think we should refactor this to use PodContainerHealthy() at this time.

The good news is that once we deliver a build that addresses issues, we can confidently abstract them into pod.go. What do you think?

Edit: Since you're more familiar with PodContainerHealthy(), do you think it would provide an improvement over the logic we have here? I meant to ask that up front.

With these PRs for the escalation's we have now recreated the logic of PodContainerHealthy(). I think it is time to look at calling this function instead of leaving the logic in RunWorkWithLogger()

Thank you for your review @lranjbar. I agree that it is a better practice to use a specific helper like PodContainerHealthy() and I see that it was added in #1371 over a month ago but was not discussed for this effort.

I've been engaged in this review and #1400, so I took a look at PodContainerHealthy to see how it could fit here. While it's a good abstraction of our operational logic prior to #1400 (with some additional checks), it's not a drop-in replacement for the logic that has been iterated on, tested, and reviewed in #1400 / #1411. At a glance, there are differences to handling exit codes and Ready that don't align. These are areas the team has focused a great deal on, and they are critical to our response here.

Our priority with this PR is to get our updated logic in a build out for testing and feedback (and be ready to incorporate that feedback). I'm totally onboard with refactoring after we confirm a fix, but before is premature in my opinion. So I don't think we should refactor this to use PodContainerHealthy() at this time.

The good news is that once we deliver a build that addresses issues, we can confidently abstract them into pod.go. What do you think?

Edit: Since you're more familiar with PodContainerHealthy(), do you think it would provide an improvement over the logic we have here? I meant to ask that up front.

It was brought up several times before any pull request was made by two team members.

The fact that you chose to dismiss that multiple times means that you made a third team member recreate the identical logic in the hero function. Instead of using the functions that were made a month ago, for this purpose.

That discussion was before #1399, #1400 and #1411 even existed. Now is the time.

The temperature is rather high on this one,

I am not seeing the advantage of delaying merging this.

We will revisit how we can refactor interacting with Pods and Containers into a more generalised solution, taking into account the work already done in that space.
Given the Sev 1 is already a week old, and we have a fully tested and reviewed PR here, I am more comfortable accepting the risk of potential code duplication rather than accepting the risk of new tests and a refactor of PodContainerHealthy()

Once we have the customers back to green we will revisit 😄